Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…into feature/logout
RitikJaiswal75
left a comment
RitikJaiswal75
left a comment
There was a problem hiding this comment.
Tag this branch with the issue.
| right: 15px; | ||
| top: 45px; | ||
| } | ||
| } |
There was a problem hiding this comment.
why are we not using rem, em here?
There was a problem hiding this comment.
Beacuse I never worked with them and thought px will do the work, can you please let me know why you are suggesting me to use rem,em or I can use %
There was a problem hiding this comment.
| isLoggedIn, | ||
| USER_PROFILE_URL, | ||
| SIGN_OUT_URL, | ||
| userData, | ||
| DEFAULT_AVATAR, |
There was a problem hiding this comment.
why are some props upper case and some camelCase?
There was a problem hiding this comment.
because both props that are used in camelCase are useState hooks. let me know If I can change it or not.
| const Dropdown = ({ | ||
| isLoggedIn, | ||
| USER_PROFILE_URL, | ||
| SIGN_OUT_URL, |
There was a problem hiding this comment.
should be coming directly from the constants file, why are we passing it as a prop?
There was a problem hiding this comment.
Because I tried to make the component re-usable.
| import Link from 'next/link'; | ||
| import styles from './dropdown.module.scss'; | ||
|
|
||
| const Dropdown = ({ |
There was a problem hiding this comment.
where are the PropTypes defined for this component?
There was a problem hiding this comment.
I guess because This is JavaScript file not a TypeScript file,I don't know that we have to define PropTypes here also, please guide me.
| const signOut = () => { | ||
| fetch(SIGN_OUT_URL, { | ||
| method: 'GET', | ||
| credentials: 'include', | ||
| }).then(() => { | ||
| window.location.reload(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
What if it fails? where are we handling the failure?
There was a problem hiding this comment.
yess got it I will look into it. thank you.
| ); | ||
| }; | ||
|
|
||
| export default Dropdown; |
There was a problem hiding this comment.
Cant, we look to create this as a generic re-usable dropdown component? so it can be used across members? Right now it's only used for Signing users out.
There was a problem hiding this comment.
Also not sure why is this named a dropdown component.
I cannot see any select attribute 🤔
There was a problem hiding this comment.
I have created a re-usable dropdown component please let me know if I made any wrong choice.
I don't name this component based on any attribute I basically named it because how it will appear in the UI as you can see it the above video, please let me know if I can change the component name from dropdown to any better name.
| </Link> | ||
| </div> | ||
| <Dropdown | ||
| isLoggedIn={isLoggedIn} |
There was a problem hiding this comment.
When will the value be false in the isLoggedIn that you are passing in Dropdown component?
Because what I see is that it is displayed only when isLoggedIn is true.
Also, there is no logical sense in importing these constant values in Navbar component and passing them in Dropdown as props, you should rather import them in Dropdown component itself.
5 participants
What is the change?
Added Sign-out option to the Navbar of the members page
Here the ticket number for the refrence #418
Is it bug?
*Dev Tested?
*Tested on:
Platforms
Browsers
Before / After Change Video.
Before Changes
As You can see in the video that before there is not an option for sign-out when user clicks on his/her name they are directed to My profile page.
screen-capture.2.webm
After Changes
As You can see in the video that after my changes there is a drop down added that opens when user clicks on his/her profile with two options 01. My profile and 02. Sign-out.
screen-capture.3.webm